Looks like there are a few issues preventing this PR from being merged!

• GitHub Actions are failing:
  • Pre-commit checks

If you'd like me to help, just leave a comment, like

@OpenHands please fix the failing actions on PR #372 at branch `openhands/swtbench-constants-refactor`

Feel free to include any additional details that might help me get this PR into a better state.

_{^{You can manage your notification settings}}

@OpenHands
/codereview-roasted
Make sure this PR doesn't alter any constant or behavior
Suggest next step refactoring for cleaner code

I'm on it! simonrosenberg can track my progress at all-hands.dev

🔴 Code Review: PR #372 - SWTBench Constants Refactor

Taste Rating: 🟡 Acceptable - Works but has one behavioral change that needs attention

[CRITICAL ISSUES] (Must fix - these break fundamental principles)

[benchmarks/swtbench/eval_infer.py, Lines 246-248] ⚠️ Breaking Change: The workers parameter type changed from str to int:

# Original:
def run_swtbench_evaluation(..., workers: str = "12") -> None:

# New:
def run_swtbench_evaluation(..., workers: int = DEFAULT_EVAL_WORKERS) -> None:

While this works at runtime (Python is dynamically typed), it's a function signature change that could break type-checked callers. The argparse also changed from accepting a string to type=int.

Recommendation: If this is intentional (improving type correctness), document it in the PR description as a breaking change. Otherwise, keep workers: str and use DEFAULT_EVAL_WORKERS_STR: Final[str] = "12".

[IMPROVEMENT OPPORTUNITIES] (Should fix - violates good taste)

[benchmarks/swtbench/constants.py, Line 35] 🔍 Magic Number: IMAGE_NAME_SEPARATOR = "1776" - This cryptic value needs a comment explaining its origin. Is it a date? A convention from SWE-bench? Future maintainers will be confused.

[benchmarks/swtbench/constants.py, Lines 38-49] 🔧 Inconsistent Enum Usage: You define BuildMode enum but then use .value everywhere:

DEFAULT_BUILD_MODE: Final[str] = BuildMode.CLI.value  # Why not just BuildMode.CLI?
BUILD_MODE_CHOICES: Final[Tuple[str, ...]] = tuple(m.value for m in BuildMode)

Consider using the enum directly and converting to string only at the argparse boundary.

[benchmarks/swtbench/constants.py, Line 13] 🔧 Inconsistent Type Definition: TargetType is a Literal while BuildMode is an Enum. For consistency, consider making TargetType an enum too, or document why they differ.

[STYLE NOTES] (Minor observations)

[benchmarks/swtbench/constants.py] 📝 The heavy use of section separators (# ====) is verbose. Consider using a single blank line between sections - the code is already well-organized.

[benchmarks/swtbench/eval_infer.py, Line 214] ✅ SETUP_FILES_TO_REMOVE changed from list to tuple - this is fine since remove_files_from_patch accepts any iterable.

VERDICT:

❌ Needs minor rework: The workers type change from str to int is a behavioral/API change that should be explicitly acknowledged or reverted.

KEY INSIGHT:

The refactoring successfully centralizes constants without changing their values, but inadvertently "improves" the workers parameter type from str to int - which, while arguably better, is a breaking change that violates the "don't alter behavior" requirement.

Suggested Next Steps for Cleaner Code:

1. Fix the workers type issue - either revert to str or document as intentional breaking change
2. Add docstring to IMAGE_NAME_SEPARATOR explaining the "1776" convention
3. Consider grouping related constants into @dataclass(frozen=True) for better organization:

   @dataclass(frozen=True)
   class DockerConfig:
       swebench_prefix: str = "docker.io/swebench/"
       swtbench_prefix: str = "docker.io/swtbench/"
       agent_server_base: str = "ghcr.io/all-hands-ai/agent-server"

4. Unify type definitions - either all Literals or all Enums for consistency
5. Consider adding a __all__ export list to make the public API explicit

View full conversation